-
Notifications
You must be signed in to change notification settings - Fork 10.4k
59462 OIDC par failure #61947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
59462 OIDC par failure #61947
Conversation
@dotnet-policy-service agree company="Tyler Technologies" |
Would love feedback from @josephdecock Also, I'm not sure if this should go into main, but I'd like to see it show up in at least the 10.0 release (it's probably too late to get it into 9). |
If there anything else I need to do process-wise to make this PR ready for review, please let me know. This is my first contribution and I might have accidentally missed a step. |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
/azp run |
Commenter does not have sufficient privileges for PR 61947 in repo dotnet/aspnetcore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new OIDC event OnPushAuthorizationFailed
to handle PAR (Pushed Authorization Request) failures during the challenge phase. When a PAR request fails with an exception, applications can now handle the response gracefully instead of receiving an unhandled middleware exception.
Key changes:
- New
PushedAuthorizationFailedContext
class to provide context for PAR failures - New
OnPushAuthorizationFailed
event inOpenIdConnectEvents
- Exception handling logic in
OpenIdConnectHandler
to catch PAR failures and fire the event - Test coverage for the new failure handling functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
OpenIdConnectChallengeTests.cs |
Adds test to verify PAR failure event handling works correctly |
PublicAPI.Unshipped.txt |
Exposes new public API members for the failure context and event |
OpenIdConnectHandler.cs |
Wraps PAR logic in try-catch to fire failure event when exceptions occur |
LoggingExtensions.cs |
Adds logging for handled PAR failure responses |
PushedAuthorizationFailedContext.cs |
New context class providing exception details and handled flag |
OpenIdConnectEvents.cs |
Adds the new failure event to the events collection |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/Security/Authentication/OpenIdConnect/src/Events/PushedAuthorizationFailedContext.cs
Outdated
Show resolved
Hide resolved
@@ -74,7 +74,7 @@ internal static partial class LoggingExtensions | |||
[LoggerMessage(37, LogLevel.Debug, "The UserInformationReceived event returned Skipped.", EventName = "UserInformationReceivedSkipped")] | |||
public static partial void UserInformationReceivedSkipped(this ILogger logger); | |||
|
|||
[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] | |||
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event ID was changed from 57 to 60, but this appears to be an existing log message that should maintain its original ID. This change could break logging consistency and any code that depends on the specific event ID.
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] | |
[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were two values set for 57, which I assumed to be a mistake from a previous commit. I updated this to be the next unique number at the time. I am not sure if that was the correct move.
src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs
Outdated
Show resolved
Hide resolved
…horizationFailedContext.cs Co-authored-by: Copilot <[email protected]>
Sorry for a hugely delayed response on this thread! I really like this idea and the PR. It's good that if the error isn't handled, the exception still bubbles up, but if you want to handle it you can. I think this approach of a separate event for pushed authorization is also the best thing to do, because the existing AuthenticationFailed event would happen at a very different point in the protocol interaction. All in all, I think this is a great improvement, and I hope it lands in .NET 10. |
src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs
Outdated
Show resolved
Hide resolved
@halter73 Got feedback and a thumbs up from an expert! Is there anything I can do on my end to help get this through? |
Add OIDC event to allow caller to react to OIDC PAR failure during challenge phase
Description
Add new OIDC OnPushAuthorizationFailed, new context for this event, logic to fire event, tests.
Problem
When a validation failure occurs during a PAR request (ex. the request includes an invalid client_id), an OpenIdConnectProtocolException is thrown. This exception bubbles up as an unhandled middleware exception.
Please see issue #59462 for slightly more details.
Goal
We need to give the application the ability to handle the response when a PAR request fails during the challenge phase. Since an exception during the challenge phase bubbles up as a middleware exception, it is difficult for the application to respond. By including a specific OIDC event, the application has the opportunity to redirect the browser to a user-friendly error page.
Example of a web app utilizing this new feature
Fixes #59462